Skip to content

Feature: URL Parameters #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 3, 2023
Merged

Conversation

michalpokusa
Copy link
Contributor

Added option for wildcard-like URLs in HTTPServer.route.

It is now possible to do something like this:

@server.route("/something/<param_1>/<param_2>")
def name_parameters(request: HTTPRequest, param_1: str,  param_2: str = None):
    ...

and access parameters inside handler function, similarly to Django and I belive Flask.

Also updated examples to show new functionality.

There is literally no change to the API so this is 100% backwards compatible, if one does not use <> in route call, nothing changes for them.

Additionally:

  • Fixed one small issue where an empty query params was created

These changes should fulfill #42.

@nak435 nak435 mentioned this pull request Mar 20, 2023

@server.route("/device/<device_id>/action/<action>")
@server.route("/device/emergency-power-off/<device_id>")
def perform_action(request: HTTPRequest, device_id: str, action: str = None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function intended to have two @server.route()s on it?

I tried testing it as was able to get successfull responses from /device/<device_id>/action/<action>

But I just get 'connection timed out' error with no real response when I try /device/emergency-power-off/<device_id>. I'm guessing it may have been leftover from some testing durrent development and can be removed? Or if it does actually belong something may need tweaked in order to make it functional.

Copy link
Contributor Author

@michalpokusa michalpokusa Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for testing it 👍

Yes, this is intentional, not a leftover. I did it to show that it is possible to have multiple routes for single handler, like Flask.

I also test it on ESP32-S2 TFT, including the /device/emergency-power-off/<device_id>.

My guess is, that error you are encountering is in fact due to the action defaulting to None, thus entering second if with device.turn_off() which is raising NotImplementedError and not returning proper HTTPResponse.

Please check if that is the case, and if yes do you think it should be changed? (I updated the example to use print instead of NotImplementedError)

It is only an example but I agree that it might be confusing.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really cool functionality. Thank you for implementing this @michalpokusa.

One question and potential change, but beyond that this change is looking good to me. I tested most of this successfully with a Feather ESP32-S2 TFT.

@michalpokusa michalpokusa requested a review from FoamyGuy March 21, 2023 19:24
@michalpokusa
Copy link
Contributor Author

@FoamyGuy Can I get an update on this PR? I already have some more functionality in mind, but I would like to finish this one first.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current version of this looks good to me. Thank you @michalpokusa

One thing I think we should do in the examples for this library is migrate to using either secrets dictionary inside of secrets.py or perhaps the even new settings.toml which is the intended place for auth details moving forward I believe. I'll open a seperate issue for that as it's not really related to this change.

@FoamyGuy FoamyGuy merged commit fee4570 into adafruit:main Apr 3, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Apr 4, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE to 10.0.0 from 9.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#184 from adafruit/dhalbert-patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer to 2.4.0 from 2.3.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_HTTPServer#43 from michalpokusa/feature/url_parameters

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
@michalpokusa michalpokusa deleted the feature/url_parameters branch April 14, 2023 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants